selection_range: use merlin enclosing query instead of shape #1368

voodoos · 2024-09-03T15:45:14Z

@awilliambauer @pitag-ha I looked at the server answer to the selectionRange query after our discussion earlier today, and I noticed that they were, in fact, duplicated ranges:

[Trace - 17:40:29] Received response 'textDocument/selectionRange - (20)' in 6ms.
Result: [
    {
        "parent": {
            "parent": {
                "parent": {
                    "parent": {
                        "parent": {
                            "parent": {
                                "parent": {
                                    "parent": {
                                        "parent": {
                                            "parent": {
                                                "range": {
                                                    "end": {
                                                        "character": 38,
                                                        "line": 968
                                                    },
                                                    "start": {
                                                        "character": 0,
                                                        "line": 0
                                                    }
                                                }
                                            },
                                            "range": {
                                                "end": {
                                                    "character": 26,
                                                    "line": 409
                                                },
                                                "start": {
                                                    "character": 0,
                                                    "line": 379
                                                }
                                            }
                                        },
                                        "range": {
                                            "end": {
                                                "character": 26,
                                                "line": 409
                                            },
                                            "start": {
                                                "character": 0,
                                                "line": 379
                                            }
                                        }
                                    },
                                    "range": {
                                        "end": {
                                            "character": 26,
                                            "line": 409
                                        },
                                        "start": {
                                            "character": 2,
                                            "line": 380
                                        }
                                    }
                                },
...
            "range": {
                "end": {
                    "character": 26,
                    "line": 409
                },
                "start": {
                    "character": 4,
                    "line": 409
                }
            }
        },
        "range": {
            "end": {
                "character": 26,
                "line": 409
            },
            "start": {
                "character": 20,
                "line": 409
            }
        }
    }
]

With that patch these are removed:

[Trace - 17:34:27] Received response 'textDocument/selectionRange - (149)' in 12ms.
Result: [
    {
        "parent": {
            "parent": {
                "parent": {
                    "parent": {
                        "parent": {
                            "parent": {
                                "parent": {
                                    "parent": {
                                        "parent": {
                                            "range": {
                                                "end": {
                                                    "character": 38,
                                                    "line": 968
                                                },
                                                "start": {
                                                    "character": 0,
                                                    "line": 0
                                                }
                                            }
                                        },
                                        "range": {
                                            "end": {
                                                "character": 26,
                                                "line": 409
                                            },
                                            "start": {
                                                "character": 0,
                                                "line": 379
                                            }
                                        }
                                    },
                                    "range": {
                                        "end": {
                                            "character": 26,
                                            "line": 409
                                        },
                                        "start": {
                                            "character": 2,
                                            "line": 380
                                        }
                                    }
                                },
...
            "range": {
                "end": {
                    "character": 26,
                    "line": 409
                },
                "start": {
                    "character": 4,
                    "line": 409
                }
            }
        },
        "range": {
            "end": {
                "character": 26,
                "line": 409
            },
            "start": {
                "character": 20,
                "line": 409
            }
        }
    }
]

Sadly vscode still feels the need to add extraneous steps, notably when there is some indentation:

Screen.Recording.2024-09-03.at.17.36.03.mov

That's quite unfortunate... but this PR at least improves the servers' answer and does remove a few no-op steps compared to the previous behaviour.

coveralls · 2024-09-03T15:51:17Z

Pull Request Test Coverage Report for Build 4581

Details

0 of 9 (0.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 21.913%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ocaml-lsp-server/src/ocaml_lsp_server.ml	0	9	0.0%

Totals
Change from base Build 4580:	0.007%
Covered Lines:	5559
Relevant Lines:	25369

💛 - Coveralls

rgrinberg · 2024-09-03T16:05:57Z

Does this work for documents that aren't well typed?

voodoos · 2024-09-03T16:12:16Z

Does this work for documents that aren't well typed?

I would expect it to behave as good (or as bad) as the current version. Looking at the Merlin implementation they do fairly similar things but shape returns too much information for our need.

voodoos · 2024-09-03T16:15:17Z

I just had this déjà-vu feeling.

I openned a very similar PR 2 years ago...

#860

rgrinberg · 2024-09-03T16:18:57Z

Yup. I just read it over and I still don't really understand how much this changes. I suppose you're confident that this is a strict improvement? A few tests would make that a lot more convincing.

To recall my main desire for this feature: the criteria for what is a "selection" should be a purely syntactic one. It would make this command very useful for moving logical chunks of code around that don't need to be typed (comments, records field definitions, match statement clauses).

voodoos · 2024-09-03T17:20:59Z

It is true that the logic is quite different, but so far I was not able to witness any difference... I will try to write more interesting tests.

The de-duplication is done explicitly in Merlin for the Enclosing command. If we want we could simply do the same thing for the Shape command.

Still, it looks like the output of the Shape command is not fitted for the selectionRange query since most of the result from Merlin is filtered out to "try to find the nearest range". The enclosing command returns only the typedtree branch containing the cursor, which seems to correspond better to what is needed.

I will write more test, but I would argue that it is a strict improvement: the code is simpler (on both sides, Merlin and OCaml-LSP), and the results do not contain duplicates anymore.

I (still) agree with the fact that the Typedtree is probably not the best way to answer these queries, but that's out of scope for fix PR which aim is to remove duplicates.

rgrinberg · 2024-09-03T17:29:27Z

No argument against this being an improvement. You would know more than me. I would like to see some more tests to make sure we are understanding if there's a change of behavior or a regression. After hover and completion, this is my most used request in LSP. I imagine other users are also relying on this request heavily. We would all welcome improvements in this functionality.

voodoos · 2024-10-01T13:12:22Z

@rgrinberg I added a few more tests and so far these changes only remove duplicate entries from the results. I also tried the feature a bit around the codebase and I am quite confident that there is no bad change in behavior. (I am not a regular user however, so I might miss some differences.) If you have some suggestion for some more precise tests cases I will be happy to add them, but right now I think this is ready to be merged.

rgrinberg

Some tests demonstrating the behavior with:

modules containing a syntax error
modules that are unable to type check

Would be appropriate I think

Co-authored-by: PizieDust <[email protected]>

voodoos · 2024-10-03T14:03:06Z

I added two more cases, one with a module with a definition that fails to type check (the return type is not the expected one), and one with an additional syntax error: the end keyword is misspelled.

In both cases the new results appear to be a strict improvement (whose main aspect is the remove of duplicates).

xvw

Top!

CHANGES: ## Features - Add custom [`ocamllsp/typeSearch`](/ocaml-lsp-server/docs/ocamllsp/typeSearch-spec.md) request (ocaml/ocaml-lsp#1369) - Make MerlinJump code action configurable (ocaml/ocaml-lsp#1376) - Add custom [`ocamllsp/jump`](/ocaml-lsp-server/docs/ocamllsp/merlinJump-spec.md) request (ocaml/ocaml-lsp#1374) ## Fixes - Fix fd leak in running external processes for preprocessing (ocaml/ocaml-lsp#1349) - Fix prefix parsing for completion of object methods (ocaml/ocaml-lsp#1363, fixes ocaml/ocaml-lsp#1358) - Remove some duplicates in the `selectionRange` answers (ocaml/ocaml-lsp#1368)

voodoos force-pushed the selectionRange--use-merlin-enclosings-query-instead-of-shape branch from 80f8871 to b7c944f Compare September 3, 2024 15:48

voodoos force-pushed the selectionRange--use-merlin-enclosings-query-instead-of-shape branch 2 times, most recently from 3abf2cd to 4236933 Compare October 1, 2024 13:08

voodoos added a commit to voodoos/ocaml-lsp that referenced this pull request Oct 1, 2024

Add changelog entry for ocaml#1368

d5f5879

rgrinberg approved these changes Oct 1, 2024

View reviewed changes

voodoos and others added 4 commits October 3, 2024 15:39

Add more tests

e5a83f8

Co-authored-by: PizieDust <[email protected]>

selection_range: use merlin enclosing query instead of shape

4d3fe5d

test: promote duplicate removal in test

198a59c

Add changelog entry for ocaml#1368

bcb5018

voodoos force-pushed the selectionRange--use-merlin-enclosings-query-instead-of-shape branch from d5f5879 to bcb5018 Compare October 3, 2024 13:42

xvw approved these changes Oct 3, 2024

View reviewed changes

voodoos merged commit 7958ae9 into ocaml:master Oct 8, 2024
5 of 7 checks passed

voodoos mentioned this pull request Nov 29, 2024

[new release] lsp (3 packages) (1.20.0) ocaml/opam-repository#26995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

selection_range: use merlin enclosing query instead of shape #1368

selection_range: use merlin enclosing query instead of shape #1368

voodoos commented Sep 3, 2024

coveralls commented Sep 3, 2024 •

edited

Loading

rgrinberg commented Sep 3, 2024

voodoos commented Sep 3, 2024 •

edited

Loading

voodoos commented Sep 3, 2024

rgrinberg commented Sep 3, 2024

voodoos commented Sep 3, 2024

rgrinberg commented Sep 3, 2024

voodoos commented Oct 1, 2024

rgrinberg left a comment

voodoos commented Oct 3, 2024 •

edited

Loading

xvw left a comment

selection_range: use merlin enclosing query instead of shape #1368

selection_range: use merlin enclosing query instead of shape #1368

Conversation

voodoos commented Sep 3, 2024

coveralls commented Sep 3, 2024 • edited Loading

Pull Request Test Coverage Report for Build 4581

Details

💛 - Coveralls

rgrinberg commented Sep 3, 2024

voodoos commented Sep 3, 2024 • edited Loading

voodoos commented Sep 3, 2024

rgrinberg commented Sep 3, 2024

voodoos commented Sep 3, 2024

rgrinberg commented Sep 3, 2024

voodoos commented Oct 1, 2024

rgrinberg left a comment

Choose a reason for hiding this comment

voodoos commented Oct 3, 2024 • edited Loading

xvw left a comment

Choose a reason for hiding this comment

coveralls commented Sep 3, 2024 •

edited

Loading

voodoos commented Sep 3, 2024 •

edited

Loading

voodoos commented Oct 3, 2024 •

edited

Loading